Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MI - dev control plus debug (#4) #788

Merged
merged 5 commits into from
Mar 23, 2023
Merged

MI - dev control plus debug (#4) #788

merged 5 commits into from
Mar 23, 2023

Conversation

rejoe2
Copy link
Contributor

@rejoe2 rejoe2 commented Mar 19, 2023

  • restructure serial debug output (partly)
  • MI - more code review
  • fix debug output for power control cmds

  • MI - limitation commands answered

  • Debug - add more macros and texts

Might as well help for #478

Unfortunately, this atm doesn't really lower used flash memory

  • MI - limit command
  • now seems to work for MI-600 (to be verified)
  • debug - more use of macros (wrt. to real effect see last remark)

* restructure serial debug output (partly) 

+ MI - more code review

* fix debug output for power control cmds

* MI - limitation commands answered

* Debug - add more macros and texts

Might as well help for #478

Unfortunately, this atm doesn't really lower used flash memory

* MI - limit command

- now seems to work for MI-600 (to be verified)
- debug - more use of macros (wrt. to real effect see last remark)
@rejoe2
Copy link
Contributor Author

rejoe2 commented Mar 19, 2023

Needs review wrt. to memory optimazation - doesn't really need less flash atm...
Maybe methods in debug need to be changed?

lumapu added a commit that referenced this pull request Mar 23, 2023
* merged MI, thx @rejoe2 #788
* fixed reboot message #793
@lumapu lumapu merged commit b498ea6 into lumapu:development03 Mar 23, 2023
@rejoe2
Copy link
Contributor Author

rejoe2 commented Mar 24, 2023

Danke für's mergen.

Das Makro DPRINTHEAD hast du nicht in hmPayload übernommen.
Frage: Ist das auch nicht geplant, oder war das einfach ein Zeit-Thema beim durchsehen? Wenn letzteres: magst du dazu einen PR haben?

@lumapu
Copy link
Owner

lumapu commented Mar 24, 2023

gerne. Das DPRINTHEAD hat aus meiner Sicht keinen Vorteil zur aktuellen Implemtierung. Daher eher nicht, außer du hast überzeugende Argumente.
Habe viel rückgängig gemacht von deinen Änderungen, da der Code von der Lesbarkeit zu stark gelitten hat und auf der anderen Seite kein messbarer Vorteil bzgl. Speicher entstanden ist.

@lumapu
Copy link
Owner

lumapu commented Mar 24, 2023

habe gerade nochmal geschaut, eigentlich nicht so blöd das Macro, allerdings brauchen wir einen anderen Namen dafür, da es für mich kein Header ist.
Das wird doch ausschließlich für das Printen der inverter ID verwendet oder?

was hälst du zB von DPRINT_IVID? müssen wir wirklich ein Level mit angeben oder einfach Standard Info-Level voraussetzen?

@rejoe2
Copy link
Contributor Author

rejoe2 commented Mar 24, 2023

Volles Verständnis für den Rückbau! Hätte auch nach meinem Eindruck nur Vorteile, wenn man das für Arduino bauen wollte und dann die Texte deutlich reduziert.

Ad DPRINT_IVID:

  • Was die Benennung angeht, bin ich völlig schmerzfrei;
  • Ja, es geht nur darum, die jeweilige Zeile "sauber" anzufangen.
  • bei den bisherigen Codes (also den dadurch ersetzten) war halt immer der Level mit dabei, und eigentlich hat mir diese "Universalität" ganz gut gefallen. (EDIT: Ist es nicht auch ein gewisser Standard bei allen DPRINT-Makros?)
  • Was "Lesbarkeit" angeht, finde ich dieses Makro eigentlich "besser" als die immer gleichen zwei Zeilen; ist aber eine Geschmacksfrage...

@lumapu
Copy link
Owner

lumapu commented Mar 24, 2023

ja dann konvertieren wir doch die immer gleichen Zeilen und verwenden dafür dass neue DPRINT_IVID, was deinem ursprünglichem DPRINTHEAD entspricht.
Schön wäre es mit Level, aber ohne das leere F("").
Würdest du das übernehmen?

@rejoe2
Copy link
Contributor Author

rejoe2 commented Mar 24, 2023

Mach mich mal dran.

Ad DPRINT und DBGPRINT: Da ist mir noch aufgefallen, dass DHEX dann wohl auch besser DBGHEX heißen sollte (EDIT: no, das wird auch von anderer Stelle her genutzt) (bzw. dasselbe für die LN-Variante) (EDIT: da schon). Da schaue ich auch durch, was da noch an "Potential" in den beiden Dateien ist, oder?

@lumapu
Copy link
Owner

lumapu commented Mar 24, 2023

gerne, stimmt bei der Benennung war ich nicht konsequent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants